Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make inplace_function SFINAE aware #155

Closed
wants to merge 1 commit into from
Closed

Make inplace_function SFINAE aware #155

wants to merge 1 commit into from
+154 −19

Conversation

Voultapher
Copy link
Contributor

@Quuxplusone @p-groarke this should address the issues described in #149.

I went with a custom implementation of std::is_invocable_r, because the standard versions don't seem to respect return types, and there seem to be pre C++17 users of this library.

@Voultapher Voultapher closed this Mar 31, 2019
@Voultapher Voultapher force-pushed the feature/make-inplace-function-sfinae-aware branch from 26fd931 to 94debf7 Compare March 31, 2019 14:56
@Voultapher Voultapher reopened this Mar 31, 2019
@Voultapher
Copy link
Contributor Author

I force pushed some indentations fixes, can someone re trigger the CI.

@Quuxplusone
Copy link
Contributor

Quuxplusone commented Mar 31, 2019 •
edited
Loading

I force pushed some indentations fixes, can someone re trigger the CI.

(Force-pushed to your feature branch, I assume and hope. ;)) My understanding is that GitHub will notice and re-trigger the Travis CI build on its own, relatively quickly, any time the tip of the feature branch changes. Whether the history "changed" or just "incremented" doesn't matter to GitHub.

EDIT: oh, I get it. If the branch is at one SHA when the CI is triggered, and then that SHA disappears from GitHub before the CI actually runs, the CI job errors out. That makes sense. And I think I found a "Restart build" button. :)

Quuxplusone
Quuxplusone requested changes Mar 31, 2019
View reviewed changes
SG14/inplace_function.h Outdated Show resolved Hide resolved
SG14/inplace_function.h Outdated Show resolved Hide resolved
SG14_test/inplace_function_test.cpp Outdated Show resolved Hide resolved
SG14_test/inplace_function_test.cpp Outdated Show resolved Hide resolved
SG14/inplace_function.h Show resolved Hide resolved
@Voultapher Voultapher force-pushed the feature/make-inplace-function-sfinae-aware branch 2 times, most recently from 04d4075 to 25a4746 Compare April 2, 2019 20:23
@Voultapher
Copy link
Contributor Author

Ok, that should address your last comment regarding the conversion tests. I went with a hybrid approach, that should catch most problems, and added some additional closure type tests.

@Voultapher Voultapher force-pushed the feature/make-inplace-function-sfinae-aware branch from 25a4746 to e08c83f Compare April 2, 2019 21:37
@Voultapher
Copy link
Contributor Author

Friendly ping, is there something blocking this?

Quuxplusone
Quuxplusone reviewed Apr 4, 2019
View reviewed changes
SG14_test/inplace_function_test.cpp Outdated Show resolved Hide resolved
SG14_test/inplace_function_test.cpp Outdated Show resolved Hide resolved
@Voultapher Voultapher force-pushed the feature/make-inplace-function-sfinae-aware branch from e08c83f to cdbe652 Compare April 6, 2019 17:13
@Voultapher
Copy link
Contributor Author

It feels like this pr is spiraling out of scope. Given that the remaining problems exist on master and are somewhat unrelated to the changes in this pr, I'd suggest merging this as is and addressing #125 in a separate pr.

@Voultapher Voultapher force-pushed the feature/make-inplace-function-sfinae-aware branch from cdbe652 to 0ba78c1 Compare April 28, 2019 10:39
@Voultapher
Copy link
Contributor Author

rebase @Quuxplusone ping

Quuxplusone
Quuxplusone approved these changes Apr 28, 2019
View reviewed changes
Copy link
Contributor

@Quuxplusone Quuxplusone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo these remaining comments.

SG14/inplace_function.h Outdated Show resolved Hide resolved
SG14_test/inplace_function_test.cpp Outdated Show resolved Hide resolved
SG14_test/inplace_function_test.cpp Outdated Show resolved Hide resolved
SG14_test/inplace_function_test.cpp Show resolved Hide resolved
@Voultapher Voultapher force-pushed the feature/make-inplace-function-sfinae-aware branch from 0ba78c1 to 4544736 Compare April 28, 2019 18:40
@Voultapher Voultapher force-pushed the feature/make-inplace-function-sfinae-aware branch from 4544736 to 62ad577 Compare April 28, 2019 18:41
@Voultapher
Copy link
Contributor Author

friendly ping @Quuxplusone

@Quuxplusone
Copy link
Contributor

Merged as a53e9e6 — thanks for your hard work!

I believe I found one more minor issue which I fixed in 98591c4.

@Quuxplusone Quuxplusone closed this May 1, 2019
@Quuxplusone Quuxplusone mentioned this pull request May 1, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Quuxplusone Quuxplusone Quuxplusone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Voultapher @Quuxplusone